review totally appreciated @zklaus - I'll address the suggestions today 🍺

OK so due to all these changes, even if the tests cover most of the thing, I retested with a comprehensive functional test (a big recipe) and all is well 👍

@zklaus @bouweandela dudes what's your say - we are starting to rely even more on this to be merged and am aware of its shortcomings and kluges but at the moment it'll prove more useful if merged than sitting in a corner - can we try and see about this? Muchos gracias 🍺

That really underscores the importance of smaller and cleaner PRs to begin with. After all, nobody needs a code review to know that 90 lines of code and 7 levels of indentation in one function are not appropriate.

Anyway, I'll copy here the open discussion from the side PR #522:

@zklaus wrote:

Next step: unify _update_fx_settings_mask and _update_fx_settings_stats.

For this it might be helpful to note that _get_spatial_stats_fx_vars really has nothing about spatial or stats things, but rather just gets the requested fx vars from the setting.

Looking at line 534 we notice that when the update method is called, the settings parameter already contains the pertinent subsection so that the manual distinction in lines 520, 524, and 528 is unnecessary.

Indeed, this automatic determination of fx variables makes one wonder if that could replace the manual listing of sft{lf,of,gif} entirely.

@valeriupredoi wrote:

but _get_spatial_stats_fx_vars needs to be called because it's the only function that gets the fx_files attr from the step (the others - mask landsea and weighted - don't need to). Kinda hard to unify the two funcs coz one need the call to get_output_file - ideas? beer

@zklaus wrote:

Well, in one we place the fx file as determined by _get_correct_fx_file in the fx_dict, in the other the result of get_output_file. What's the reason for this difference?

@valeriupredoi wrote:

get_output_file is used as a patch to get the already preprocessed and written to disk fx file, since the preprocessing and file writing is done independently of the whole fx workflow and is done like for any other variable

@zklaus wrote:

But why is that needed for some files, but not for others? What if a custom order is applied and both mask and area files have been regridded, for instance?

But why is that needed for some files, but not for others? What if a custom order is applied and both mask and area files have been regridded, for instance?

That is a very good question! The workflow for fx files that are used for area/volume/zonal_statistics is fundamentally different and herein lies the tricksy nature of this PR (well, the part of it that deals with preprocessing these data):

• the input fx data needed for those preprocessors are preprocessed entirely and firstly (as ancestors to the main variable) and these preprocessed files are written to disk, the very first thing the workflow does;
• then, and only then, after all work's been done on those files only do the area/volume/zonal_statistics preproc step should look for files for fx_files - and those are U-turned from the get_output_files() assuming (and knowing) that they are there

This is a kludge - just because variable-fxvariable coupling is unavailable in a true sense now, given the current architecture. @bouweandela and myself we can work next using iris' new cell_measure functionality so we can do this in an elegant and practical way. Note that this method is relatively foolproof ie it is not prone to fail unexpectedly - if it fails, it fails due to an issue in the preprocessing step that's easy to trace.

About regridding - at the moment both the mask and model data will be regridded which is bad, but then again we can always put the area/volume/zonal_statistics preprocessor before the regridding step; we can also include the regrid as a prohibited step for the fx data just as are the time preproc steps. I left that out so I can talk to you about it - and here we are, talking about it 😁

also to note: the preprocessor steps applied to fx data are a subset of the main variable they're associated with: [step1, step2...stepN] where stepN is the last step just before area/volume/zonal_statistics, everything that follows after that for the main var preprocessor is not applied to the fx variable; also step1-N do not include any time preprocessors

Really like these last changes at a glance! Will do in-depth review tomorrow. Of course, the for-loop needs to be turned into a dict comprehension.

Really like these last changes at a glance! Will do in-depth review tomorrow. Of course, the for-loop needs to be turned into a dict comprehension.

cheers dude, done it, and also simplified and added test - now the user can define his own fx variables no matter what mask preproc

this is a terribly outdated branch, together with the other sisters, they awere useful to @ledm and @LisaBock I believe, but I think it's about time to retire them (close them) - what do you think @zklaus and @bouweandela - do we maybe want to recycle some of this functionality towards implementing the cell measures stuffs with new iris?

Maybe keep it around until #999 is merged?

I believe we can close this since #999 got merged, can't we? @valeriupredoi

Since V wanted to close them for a long time, I'll do it now. @valeriupredoi, @bouweandela please reopen if you disagree.